Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

event name & listener callback types #53642

Merged
merged 3 commits into from
Nov 25, 2024

Conversation

rudiedirkx
Copy link
Contributor

@rudiedirkx rudiedirkx commented Nov 23, 2024

$callback type is currently QueuedClosure|\Closure|string|array, which includes a lot, but not invokable classes. I changed it to QueuedClosure|class-string|array|callable which does include invokable classes (callable), and is more specific about the string (class-string). I'm not sure though. Let's run some tests.

@taylorotwell taylorotwell merged commit 3f484c4 into laravel:11.x Nov 25, 2024
38 checks passed
@einar-hansen
Copy link
Contributor

einar-hansen commented Nov 27, 2024

The changes to the listen method on the Dispatcher class is making PHPStan complain, are you certain about this change @rudiedirkx? The second parameter should/could allow for a string (not limited to class-string), since it's possible to invoke a method on a class using a string. Example from a subscriber class that I have:


public function deleting(ProductDeletingEvent $event): void
{
   ...code
}

public function subscribe(Dispatcher $dispatcher): void
{
    $dispatcher->listen(
        ProductDeletingEvent::class,
        self::class.'@deleting',
    );
}

@rudiedirkx
Copy link
Contributor Author

@einar-hansen I would listen like this:

$dispatcher->listen(ProductDeletingEvent::class, self::deleting(...));

instead of the @, but you are right, string should still be a valid $callback, because Laravel accepts the @ syntax. I never use that, so my stan didn't complain, and I do use invokable classes, so my stan got better.

@taylorotwell This should be improved or reverted. class-string and string are both valid $callback, not just class-string.

@einar-hansen Or a new PR that improves this one? :)

@einar-hansen
Copy link
Contributor

Thanks @rudiedirkx.

I ended up updated my code to match the example in the Laravel docs, so I'm fine. I'll send in PR when things calm down around Christmas time. Thanks for a good PR 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants